fix(email): add SMTP client timeouts to prevent indefinite hanging#51
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves IAM’s SMTP email delivery robustness by adding explicit timeouts/deadlines to prevent the TLS email-sending path from hanging indefinitely during network stalls.
Changes:
- Added SMTP dial and overall-operation timeout constants.
- Updated TLS SMTP sending to use a context-bounded deadline plus TCP connection deadlines.
- Adjusted header formatting to ignore
fmt.Fprintf’s return values.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // SMTP client timeouts. Kept tight so failures surface fast and never block the request path. | ||
| const ( | ||
| smtpDialTimeout = 10 * time.Second | ||
| smtpOverallTimeout = 30 * time.Second | ||
| ) |
There was a problem hiding this comment.
The comment claims the SMTP timeouts "never block the request path", but the code can still block the caller for up to smtpOverallTimeout (and up to smtpDialTimeout during connect). Consider rewording to reflect that the goal is to bound/limit blocking time rather than eliminate it entirely.
| } | ||
|
|
||
| client, err := smtp.NewClient(conn, s.cfg.SMTPHost) | ||
| if err != nil { |
There was a problem hiding this comment.
If smtp.NewClient returns an error, the underlying conn is not closed, which can leak file descriptors on handshake/banner failures. Close conn on this error path (or defer conn.Close immediately after DialContext and cancel the defer when the client takes ownership).
| if err != nil { | |
| if err != nil { | |
| _ = conn.Close() |
| func (s *Service) sendTLS(ctx context.Context, addr string, auth smtp.Auth, to, msg string) error { | ||
| // Enforce a bounded overall deadline so no SMTP step can hang indefinitely. | ||
| ctx, cancel := context.WithTimeout(ctx, smtpOverallTimeout) | ||
| defer cancel() | ||
|
|
There was a problem hiding this comment.
Timeouts are only enforced in sendTLS; the non-TLS path still uses smtp.SendMail (which dials without timeouts) and can still hang indefinitely. If the intent is truly to ensure email sending never hangs, consider adding the same dial/deadline approach to the non-TLS path (custom net.Dialer + conn.SetDeadline + smtp.NewClient).
Description
This pull request improves the reliability and robustness of the SMTP email sending service by introducing explicit connection and operation timeouts, ensuring that email sending cannot hang indefinitely and will fail fast in case of network issues. Additionally, minor code improvements were made for error handling.
Type of Change
Service(s) Affected
Changes Made
Timeout and reliability improvements:
smtpDialTimeoutandsmtpOverallTimeout, to enforce strict timeouts for SMTP connection establishment and overall operation duration, preventing hangs in the email sending path (service.go).sendTLSmethod to use a context with a bounded timeout (smtpOverallTimeout) and set a dialer timeout (smtpDialTimeout) for connecting to the SMTP server, ensuring both connection and operation are time-limited (service.go). [1] [2]service.go).Code quality improvements:
sendmethod to ignore errors fromfmt.Fprintf, as failures there are non-critical (service.go).Lint & Build
golangci-lint run ./...passesgo build ./...succeedsgo test -race ./...passesPre-merge Checklist